Repository navigation
Conversation
commit: |
|
|
A one-time behavioral probe could also work i think. |
Do you mean to avoid accessing the private I'll think about how to expose this state, but it will of course only be usable in new versions. There isn't a strong reason for having this state/property private. (I tend to make new properties private until there is a demand for them, to simplify internal changes and keep the API smaller.) |
|
Thanks for working on this! Would you like a review? |
Yes, please!🙌🏼 |
|
Working my way slowly through the new code. Is the goal for now to leave the old code largely untouched and behaviour unchanged unless
|
|
|
||
| export type OptionLookupMode = 'parent-first' | 'local'; | ||
|
|
||
| const optionLookupModes = new WeakMap<RootCommand, OptionLookupMode>(); |
There was a problem hiding this comment.
The code is using this WeakMap to lookup a value being held separate from the RootCommand. Is there a reason for that, rather than just having a property on the RootCommand? (It isn't wrong but I am wondering if this pattern means I didn't understand something, or might be a left-over from an early approach when it was needed.)
(In Commander technically the lookup mode can be set on any command in hierarchy and not just the RootCommand, but in practice usually set the same on all commands. I am fine with only being set on RootCommand.)
| .command('serve') | ||
| .description('Serve the app') | ||
| .addOption( | ||
| new Option('-c, --leaf-choice <value>', 'Leaf choice').choices([ |
There was a problem hiding this comment.
This is checking terminology and intent, but not wrong as such. I use "leaf" to refer to the final subcommand, and would use nested to refer to a "middle" command. The command hierarchy here is commander-option-scope serve nested. From the naming, I would have expected commander-option-scope nested serve.
Most of the tests are using serve and I suspect you may have started with just one layer of commands and serve was the leaf. 😄
Could maybe rename the leaf-* options to serve-* and avoid potential for leaf confusion.
|
I have carefully read through the new code and makes sense. I like I'll work through the tests and do some runtime manual tests too to confirm coverage. I was just taking a light look at the fixture and can see some thoughtful layers of options for probes! |
related to the discussions in #154